-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add a subset of properties for stackset stack manipulation #120
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arrjay thanks for this! Just 1 minor comment and then we are good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also forgot to mention tests. Can you add a test to assert that these settings take effect?
+1 for this feature. |
Co-authored-by: Cory Hall <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get this merged it needs the following:
- README update
- unit tests
- merge conflicts addressed
Please ping me if you need another review. Since this PR has been open for so long I would be happy if anyone from the community decided to take this one across the finish line. Cheers!
Addresses #116
the current implementation of this provides some known properties that are ok to pass along, rather than everything except the
synthesizer
property.